Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkp/pkp-lib#10671 Review Report plugin compatibility and test for 3.5.0 #55

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

touhidurabir
Copy link
Member

@touhidurabir
Copy link
Member Author

@asmecher can you please review this PR , as per my check it works fine with latest main of OJS and OMP . This PR does not fix anything but just do some general cleanup and added some type hints .

Copy link
Member

@asmecher asmecher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This plugin could still use a lot of cleanup and modernization, but no special need to do that now.

@@ -150,7 +148,7 @@ public function display($args, $request)
/** @var ReviewFormElementDAO */
$reviewFormElementDao = DAORegistry::getDAO('ReviewFormElementDAO');

foreach ($reviewsIterator as $row) {
foreach ($reviewsIterator as $row) { /** @var stdClass $row */
if (substr($row->date_response_due, 11) === '00:00:00') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably out of scope, but there are some proposals for improvements to date handling over here. Long story short: as much as possible, we should use objects for date representations (and Laravel makes this easy by getting them right from the database that way). This probably still works, but it's risky because it assumes a lot about the date formats.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats right , but our review related table still hasn't been converted to Eloquent model and using DB call will also return simple object so we will not get carbon instances for those yet . However we can convert it to carbon to make it work. Should we do it here or later when we moved to Eloquent for review related tables ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants